Skip to content

MON-4625: add support for the nvmesubsystem collector - #2960

Open
simonpasquier wants to merge 1 commit into
openshift:masterfrom
simonpasquier:MON-4625
Open

MON-4625: add support for the nvmesubsystem collector#2960
simonpasquier wants to merge 1 commit into
openshift:masterfrom
simonpasquier:MON-4625

Conversation

@simonpasquier

Copy link
Copy Markdown
Contributor

No description provided.

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 29, 2026
@openshift-ci-robot

openshift-ci-robot commented Jul 29, 2026

Copy link
Copy Markdown

@simonpasquier: This pull request references MON-4625 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set.

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci

openshift-ci Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Hello @simonpasquier! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 33b1a8e2-8117-4ae1-9038-bb1555f5d036

📥 Commits

Reviewing files that changed from the base of the PR and between 98a9790 and a694cf3.

⛔ Files ignored due to path filters (7)
  • config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings.crd.yaml is excluded by !**/zz_generated.crd-manifests/*
  • config/v1alpha1/zz_generated.deepcopy.go is excluded by !**/zz_generated*
  • config/v1alpha1/zz_generated.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
  • config/v1alpha1/zz_generated.model_name.go is excluded by !**/zz_generated*
  • config/v1alpha1/zz_generated.swagger_doc_generated.go is excluded by !**/zz_generated*
  • openapi/generated_openapi/zz_generated.openapi.go is excluded by !openapi/**, !**/zz_generated*
  • openapi/openapi.json is excluded by !openapi/**
📒 Files selected for processing (2)
  • config/v1alpha1/types_cluster_monitoring.go
  • payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
  • payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml
  • config/v1alpha1/types_cluster_monitoring.go

📝 Walkthrough

Walkthrough

Adds optional nvmExpressSubsystem configuration to the ClusterMonitoring API. The configuration requires a collectionPolicy set to Collect or DoNotCollect. The collector is enabled when the field is omitted. The ClusterMonitoring CRD schema defines the field and validates the policy values.

Possibly related PRs

  • openshift/api#2948: Adds related node-exporter collector configuration fields and CRD schemas.

Suggested reviewers: joelspeed

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive No pull request description was provided, so its relation to the changeset cannot be assessed. Add a short description that explains the new NVMe subsystem collector configuration and collection policy.
✅ Passed checks (14 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the addition of support for the NVMe subsystem collector.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed The PR changes only API types, generated schemas, and OpenAPI files; no test files or Ginkgo test declarations were added or modified.
Test Structure And Quality ✅ Passed The PR changes only API types and generated schemas; no *_test.go files or Ginkgo It blocks are added or modified, so this test-quality check is not applicable.
Microshift Test Compatibility ✅ Passed The commit changes only configuration, generated OpenAPI, and CRD files; no Ginkgo tests or test-like paths were added, so MicroShift compatibility checks do not apply.
Single Node Openshift (Sno) Test Compatibility ✅ Passed The patch adds API types and generated schemas only; it adds no Ginkgo e2e tests or test files, so SNO multi-node compatibility checks do not apply.
Topology-Aware Scheduling Compatibility ✅ Passed The PR changes API types, generated schemas, deepcopy, and OpenAPI files only; it adds no scheduling constraints, replicas, affinity, tolerations, selectors, or workload manifests.
Ote Binary Stdout Contract ✅ Passed The commit changes API/schema and generated metadata only; added Go code contains no fmt/log/klog/os.Stdout writes or process-level suite entry points.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed The PR changes Go API declarations, generated schemas, and OpenAPI files only; no Ginkgo e2e test files or test declarations were added, so this check does not apply.
No-Weak-Crypto ✅ Passed No weak cryptography usage detected. PR adds NVMe subsystem collector configuration only; no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB or custom crypto implementations present.
Container-Privileges ✅ Passed The PR adds Go types and CRD schema fields only; no added manifest lines contain privileged, hostPID, hostNetwork, hostIPC, SYS_ADMIN, root, or allowPrivilegeEscalation settings.
No-Sensitive-Data-In-Logs ✅ Passed The commit adds only NVMe collector types, schemas, OpenAPI metadata, and deepcopy code; added lines contain no logging calls or sensitive values.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented
The command is terminated due to an error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented


Comment @coderabbitai help to get the list of available commands.

@openshift-ci openshift-ci Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 29, 2026
@qodo-for-rh-openshift

Copy link
Copy Markdown

PR Summary by Qodo

Add ClusterMonitoringConfig support for node-exporter nvmesubsystem collector

✨ Enhancement ⚙️ Configuration changes 🕐 20-40 Minutes

Grey Divider

AI Description

• Add nvmExpressSubsystem collector configuration to NodeExporterCollectorConfig API.
• Regenerate CRD schemas to expose the new field in ClusterMonitoringConfig.
• Update OpenAPI/Swagger artifacts to include the new collector config type.
Diagram

graph TD
U["Cluster admin"] --> CMC["ClusterMonitoringConfig"] --> OP["Monitoring Operator"] --> NE["node-exporter nvmesubsystem"]
API["API types (Go)"] --> GEN["Code generation"] --> CRD["CRD manifests"]
GEN --> OAPI["OpenAPI/Swagger"]
CMC -. "validated by" .-> CRD
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Generic collector map/extension mechanism
  • ➕ Fewer per-collector API additions/regenerations
  • ➕ Allows enabling new node-exporter collectors without API changes
  • ➖ Weaker schema validation and documentation
  • ➖ Harder to enforce stable field names/semantics
  • ➖ Less discoverable for users via CRD/OpenAPI
2. Rely solely on platform defaults (no explicit API knob)
  • ➕ No API surface growth
  • ➕ Avoids shipping additional CRD/OpenAPI changes
  • ➖ Users cannot disable/enable the collector explicitly
  • ➖ Harder to manage cardinality/performance tradeoffs and compliance needs

Recommendation: The PR’s typed, per-collector configuration is the best fit for a stable, discoverable API: it preserves strong validation and consistent semantics across collectors. A generic extension point would reduce churn but would significantly degrade UX and validation for a core, user-facing CRD.

Files changed (9) +172 / -1

Enhancement (1) +21 / -0
types_cluster_monitoring.goAdd NVMExpressSubsystem collector config type and field +21/-0

Add NVMExpressSubsystem collector config type and field

• Extends NodeExporterCollectorConfig with a new nvmExpressSubsystem field and introduces NodeExporterCollectorNVMExpressSubsystemConfig with a required collectionPolicy to enable/disable the nvmesubsystem collector.

config/v1alpha1/types_cluster_monitoring.go

Documentation (1) +10 / -0
zz_generated.swagger_doc_generated.goAdd Swagger documentation for nvmExpressSubsystem and its type +10/-0

Add Swagger documentation for nvmExpressSubsystem and its type

• Extends NodeExporterCollectorConfig SwaggerDoc map with nvmExpressSubsystem and adds a dedicated SwaggerDoc map/function for NodeExporterCollectorNVMExpressSubsystemConfig.

config/v1alpha1/zz_generated.swagger_doc_generated.go

Other (7) +141 / -1
0000_10_config-operator_01_clustermonitorings.crd.yamlExpose nvmExpressSubsystem in generated ClusterMonitoringConfig CRD schema +22/-0

Expose nvmExpressSubsystem in generated ClusterMonitoringConfig CRD schema

• Adds the nvmExpressSubsystem object to the CRD’s OpenAPI v3 schema, including description, required collectionPolicy, and allowed enum values.

config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings.crd.yaml

zz_generated.deepcopy.goRegenerate deepcopy for new NVMExpressSubsystem config +17/-0

Regenerate deepcopy for new NVMExpressSubsystem config

• Updates NodeExporterCollectorConfig deepcopy to copy the new field and adds DeepCopy/DeepCopyInto implementations for NodeExporterCollectorNVMExpressSubsystemConfig.

config/v1alpha1/zz_generated.deepcopy.go

ClusterMonitoringConfig.yamlUpdate featuregated CRD manifest with nvmExpressSubsystem schema +22/-0

Update featuregated CRD manifest with nvmExpressSubsystem schema

• Mirrors the CRD schema addition for nvmExpressSubsystem in the featuregated manifest variant.

config/v1alpha1/zz_generated.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml

zz_generated.model_name.goRegister OpenAPI model name for NVMExpressSubsystem config +5/-0

Register OpenAPI model name for NVMExpressSubsystem config

• Adds OpenAPIModelName for NodeExporterCollectorNVMExpressSubsystemConfig so it can be referenced in generated OpenAPI definitions.

config/v1alpha1/zz_generated.model_name.go

zz_generated.openapi.goAdd OpenAPI schema definition and reference for NVMExpressSubsystem config +31/-1

Add OpenAPI schema definition and reference for NVMExpressSubsystem config

• Registers the new model in GetOpenAPIDefinitions, wires it into the NodeExporterCollectorConfig schema properties, and adds the schema generator for NodeExporterCollectorNVMExpressSubsystemConfig (including enum validation).

openapi/generated_openapi/zz_generated.openapi.go

openapi.jsonRegenerate OpenAPI JSON with nvmExpressSubsystem field and definition +22/-0

Regenerate OpenAPI JSON with nvmExpressSubsystem field and definition

• Adds the nvmExpressSubsystem property to the NodeExporterCollectorConfig definition and introduces the new NodeExporterCollectorNVMExpressSubsystemConfig definition with required collectionPolicy and enum constraints.

openapi/openapi.json

0000_10_config-operator_01_clustermonitorings.crd.yamlShip updated ClusterMonitoringConfig CRD in payload manifests +22/-0

Ship updated ClusterMonitoringConfig CRD in payload manifests

• Propagates the generated CRD schema change into the payload-manifests copy so clusters receive the new nvmExpressSubsystem field in the published CRD.

payload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml

@openshift-ci
openshift-ci Bot requested review from JoelSpeed and everettraven July 29, 2026 14:01
@qodo-for-rh-openshift

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

@everettraven everettraven left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2026
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e-aws-ovn
/test e2e-aws-ovn-hypershift
/test e2e-aws-ovn-hypershift-conformance
/test e2e-aws-ovn-techpreview
/test e2e-aws-serial-1of2
/test e2e-aws-serial-2of2
/test e2e-aws-serial-techpreview-1of2
/test e2e-aws-serial-techpreview-2of2
/test e2e-azure
/test e2e-gcp
/test e2e-upgrade
/test e2e-upgrade-out-of-change
/test minor-e2e-upgrade-minor

@openshift-ci

openshift-ci Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: everettraven

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 29, 2026
@simonpasquier

Copy link
Copy Markdown
Contributor Author

/retest-required

@simonpasquier

Copy link
Copy Markdown
Contributor Author

/verified by @simonpasquier

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Aug 4, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@simonpasquier: This PR has been marked as verified by @simonpasquier.

Details

In response to this:

/verified by @simonpasquier

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@simonpasquier

Copy link
Copy Markdown
Contributor Author

/skip
/retest-required

@everettraven

Copy link
Copy Markdown
Contributor

Known issue for the hypershift test that is being resolved.

/override-sticky ci/prow/verify-hypershift-integration

@openshift-ci

openshift-ci Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

@everettraven: Overrode contexts on behalf of everettraven: ci/prow/verify-hypershift-integration

These overrides will persist across retests on the current HEAD SHA. Pushing a new commit will clear them. Use /override-cancel to remove them.

Details

In response to this:

Known issue for the hypershift test that is being resolved.

/override-sticky ci/prow/verify-hypershift-integration

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@simonpasquier

Copy link
Copy Markdown
Contributor Author

/retest-required

@simonpasquier

Copy link
Copy Markdown
Contributor Author

@everettraven for some reason the bot didn't ack your override

@JoelSpeed

Copy link
Copy Markdown
Contributor

/override-sticky ci/prow/verify-hypershift-integration

@openshift-ci

openshift-ci Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/verify-hypershift-integration

These overrides will persist across retests on the current HEAD SHA. Pushing a new commit will clear them. Use /override-cancel to remove them.

Details

In response to this:

/override-sticky ci/prow/verify-hypershift-integration

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Signed-off-by: Simon Pasquier <spasquie@redhat.com>
@openshift-ci-robot openshift-ci-robot removed the verified Signifies that the PR passed pre-merge verification criteria label Aug 5, 2026
@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2026
@openshift-ci

openshift-ci Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

New changes are detected. LGTM label has been removed.

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@simonpasquier

Copy link
Copy Markdown
Contributor Author

/verified by @simonpasquier

the PR needs another lgtm because it had to be rebased.

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Aug 5, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@simonpasquier: This PR has been marked as verified by @simonpasquier.

Details

In response to this:

/verified by @simonpasquier

the PR needs another lgtm because it had to be rebased.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci

openshift-ci Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

@simonpasquier: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/verify-hypershift-integration a694cf3 link true /test verify-hypershift-integration

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants